Shouldn't LeaseList be a Linearizable read (preserve read commited)?

@serathius not as implemented in v3 afaict -

I think we should make the LeaseList as a Linearizable request, so this test case successfully detected the unexpected behavior. cc @ptabor

LeaseLeases is the only Lease API we have that doesn't get forwarded to the leader in some way, so that would make sense.

I assume it wasn't in the first place because it's a mostly informational api that's easy to overlook, but I can't find more discussion around it in the initial PR than this (#8358 (comment)).

As for how to make this Linearizable, it looks like that should be fairly easy, just a little bit weird because the LeaseHTTP package has a couple of different ways of doing things. I can open a PR if that's what we'd rather do instead (but that might have to wait for 3.6)

Agree that intuitively LeaseLeasesRequest should be 'linearizable'

I think it's pretty serious change to make it as part of stable v3.* line and violating formal backward compatibility guarantees to change the default.

I would:

1. Add bool linearizable = 1 [(versionpb.etcd_version_field)="3.6"]; to the LeaseLeases method (default false)
2. Mark it as 'deprecated'
3. Add a new method ListLeasesmethod with serializable field (default false):
   

   etcd/api/etcdserverpb/rpc.proto

   Line 453 in 3d3c437

   bool serializable = 7;

This way we would cleanup also the naming mistake (I assume) and have safe default going forward.

@ptabor Sounds good - I can open a PR for that soon. Do we wanna merge this to stabilize CI in the meantime (given that I assume a feature PR will take longer to land, and this is currently the flakiest etcd test)

I think it makes more sense to skip the test case for now instead of delivering a fix for an unexpected behavior. Once the formal fix/enhancement is ready, we can enable it again.

@ptabor Does adding linearizable field to LeaseLeases benefit us in any way when we will still introduce ListLeases? In both cases users will still take an action to get linealizable read (either set field or change method used). I think that adding the field will slow down support of new method. I think that we can skip adding linearizable field.

@endocrimes @ahrtr For now we are only deprecating LeaseLeases, we need to maintain the tests until we remove it. As so we still need to make this test stable.

Codecov Report

Merging #13868 (f71196d) into main (6c974a3) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13868      +/-   ##
==========================================
+ Coverage   72.51%   72.56%   +0.05%     
==========================================
  Files         468      468              
  Lines       38255    38255              
==========================================
+ Hits        27739    27760      +21     
+ Misses       8734     8718      -16     
+ Partials     1782     1777       -5     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c974a3...f71196d. Read the comment docs.

@ptabor Does adding linearizable field to LeaseLeases benefit us in any way when we will still introduce ListLeases? In both cases users will still take an action to get linealizable read (either set field or change method used). I think that adding the field will slow down support of new method. I think that we can skip adding linearizable field.

Seems good.

@endocrimes @ahrtr For now we are only deprecating LeaseLeases, we need to maintain the tests until we remove it. As so we still need to make this test stable.